-
Notifications
You must be signed in to change notification settings - Fork 145
Conversation
RUN mkdir -p /usr/src/app | ||
WORKDIR /usr/src/app | ||
|
||
# Install deps | ||
RUN apt-get update | ||
RUN apt-get -y install libsnappy-dev gcc g++ cmake | ||
|
||
ARG GITREF=interop | ||
|
||
RUN git clone https://github.com/ethereum/trinity.git . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this will build the master branch Trinity, instead of PR branch of Trinity. Not sure if this is what we want.
If we want to build the PR branch, we should use something like COPY . trinity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read some docker best practice articles earlier today. An argument is that git clone
gives us some flexibility at the development stage, and COPY
is more common in production.
I think at the development stage, using git clone
makes it a more isolated environment, which is good in some way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to build the PR branch, we should use something
That's an interesting point though. The Eth1 Dockerfile uses COPY
and I actually wanted to change it to use git clone
but your point makes me re-think that. We had a few times where the docker build exposed a general problem with Trinity (usually dependency issues) and as you say, if you use git clone
then the CI run ends up always using the source from master
which sounds like a weakness.
Another possible idea would be to pre-process the Dockerfile
to replace the $GITREF
like we do with the DappNode
build.
Lines 89 to 92 in 0e3e67d
create-dappnode-image: clean | |
sed -i -e 's/ARG GITREF=\w*/ARG GITREF=$(trinity_version)/g' ./dappnode/build/Dockerfile | |
cd ./dappnode && dappnodesdk increase $(dappnode_bump) | |
cd ./dappnode && dappnodesdk build |
docker/beacon.Dockerfile
Outdated
EXPOSE 30303 30303/udp | ||
# Trinity shutdowns aren't yet solid enough to avoid the fix-unclean-shutdown | ||
ENTRYPOINT trinity $EXTRA_OPTS fix-unclean-shutdown && trinity-beacon $EXTRA_OPTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pointing out that this removes the fix-unclean-shutdown
but keeps the comment above that explains it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference on how the Eth2 Dockerfile should be organized (e.g. git clone
vs COPY
) and this doesn't touch the Eth1 Dockerfile apart from moving it so 👍 from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now. Only a nitpick.
Co-Authored-By: Chih Cheng Liang <chihchengliang@gmail.com>
What was wrong?
How was it fixed?
./docker
folderDockerfile.eth
tobeacon.Dockerfile
. Reasons:.Dockerfile
file name for GitHub/editor highlight. ✨beacon.Dockerfile
and add CI test.The following command works for me to:
docker build -t="ethereum/trinity-beacon" -f ./docker/beacon.Dockerfile ./docker/ docker run ethereum/trinity-beacon -l=DEBUG interop --validators=0,1,2 --start-delay=10 --wipedb
p.s. I believe that a single beacon node can't finalize block because it doesn't import the attestaton to the pool. The fix is in #1311.
To-Do
Cute Animal Picture